internal/bootstrap/bootstrap.go (2)

43-46: 0755 on ~/.airlock — confirm intent.

The config dir will hold sandboxes.json and mounts.json, which may contain host-path mount mappings and sandbox metadata. World-readable (0755) is defensible for a dotdir but consider 0700 since this is per-user state and nothing external needs to traverse it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bootstrap/bootstrap.go` around lines 43 - 46, The configDir creation
uses os.MkdirAll with mode 0755 which makes ~/.airlock world-readable; change
the permission to 0700 to restrict access to the user only. Update the
os.MkdirAll call that creates configDir (the variable named configDir in
bootstrap.go) to use FileMode 0700 so sandboxes.json and mounts.json remain
private.

---

38-88: Assemble is untestable without Lima on the host, and fails early for init wizard users.

Two related concerns:

1. Testability (root cause of the CI failures in bootstrap_test.go). Assemble constructs lima.NewLimaProvider() unconditionally. Any caller — test or otherwise — needs limactl on PATH. Consider introducing a small factory/options seam so tests (and future non-Lima providers) can swap it:

   type Options struct {
       ProviderFactory func() (api.Provider, error) // default: lima.NewLimaProvider
       // ...
   }
   func Assemble(opts ...Option) (*Dependencies, error) { ... }

2. UX on airlock init. The PR wires the interactive wizard through bootstrap.Assemble(). If a first-time user runs airlock init before installing Lima, they'll hit init lima provider: limactl not found before ever seeing the wizard — despite the wizard not actually needing a live provider to write airlock.toml. Consider deferring provider construction until a command that truly needs it (lazy init), or letting init run against a partial Dependencies.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bootstrap/bootstrap.go` around lines 38 - 88, Assemble currently
constructs lima.NewLimaProvider() unconditionally which makes Assemble
untestable and causes init to fail when Lima/limactl is absent; change Assemble
to accept an Options/functional option (e.g., type Options struct {
ProviderFactory func() (api.Provider, error) } with a default ProviderFactory =
lima.NewLimaProvider) or an Option func to override the provider creation, and
defer provider construction for commands that don't need a live provider (allow
Assemble to return a partial Dependencies without calling ProviderFactory when a
flag/option like LazyProvider or skipProvider is set); update callers/tests to
pass a test factory or set the lazy flag so tests and airlock init can run
without limactl.

internal/bootstrap/bootstrap_test.go (1)

48-57: Nit: drop the explicit types on the interface assertion block.

staticcheck (QF1011) flags these; the RHS already carries the interface type from Dependencies. Either drop the type annotations or, better, keep them since they are precisely the LSP guard the comment advertises — in that case, add a //nolint:staticcheck // intentional interface pin to silence the linter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bootstrap/bootstrap_test.go` around lines 48 - 57, The interface
assertion block using explicit types (e.g., _ api.SandboxManager = deps.Manager,
_ api.Provider = deps.Provider, etc.) is being flagged by staticcheck QF1011; to
keep the explicit LSP-style interface pins, add a linter suppression comment
above the block: //nolint:staticcheck // intentional interface pin, so the
intent is documented and the linter is silenced; alternatively, remove the
explicit interface types and use unnamed assertions (_ = deps.Manager, _ =
deps.Provider, ...) if you prefer to avoid the nolint.

internal/config/config.go (1)

219-233: nil vs empty-slice semantics are footguns.

ValidateDynamic treats nil as "skip" but an empty non-nil slice as "nothing is valid → reject everything non-empty". Given the CLI builds profileNames/runtimeTypes via append on a nil start (see cmd/airlock/cli/cli.go), a registry with zero registrations yields nil (skip) rather than "empty list" (reject) — arguably the wrong direction: an empty registry should probably reject unknown values, not silently accept them.

Consider either documenting this explicitly (done partially in the comment) or switching to a sentinel like profileNames == nil && !checkProfiles so the contract isn't coupled to Go's nil-vs-empty-slice distinction. Low priority while the registries are always populated, but worth a note before third parties call this.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/config.go` around lines 219 - 233, ValidateDynamic's current
nil-vs-empty-slice semantics are ambiguous; change its signature to explicitly
indicate whether to perform checks (e.g., func ValidateDynamic(cfg Config,
profileNames []string, runtimeTypes []string, checkProfiles bool, checkRuntimes
bool) error) and then: if checkProfiles is true validate cfg.Security.Profile
against profileNames (treating an empty non-nil profileNames as "no valid
profiles" and reject unknown values), otherwise skip profile validation;
similarly use checkRuntimes and runtimeTypes for runtime validation. Update
callers (e.g., where profileNames/runtimeTypes are built in
cmd/airlock/cli/cli.go) to pass the appropriate checkProfiles/checkRuntimes
booleans so the contract no longer relies on nil vs empty-slice.

internal/config/save.go (1)

38-71: Section injection via strings.Split(..., "\n\n") is brittle.

This assumes WriteTOML emits exactly one blank line between every top-level table and never internally. Any of the following will break comment placement or omit comments entirely:

• A section produced without a preceding blank line (merges two sections into one chunk → only the first gets a comment).
• Inline-table or array-of-tables formatting that embeds a blank line inside a "section".
• A trailing newline producing an empty final chunk (currently skipped, but reorders the last separator).
• Any future change to the TOML serializer’s formatting would silently alter the output.

Consider a more structural approach: walk the TOML line-by-line, detect [section] headers, and prepend the comment for that section. Something like:

♻️ Proposed line-based injection

-	// Process TOML content section by section, adding comments
-	sections := strings.Split(tomlStr, "\n\n")
-	for i, section := range sections {
-		if strings.TrimSpace(section) == "" {
-			continue
-		}
-
-		// Add section comment based on section header
-		sectionName := extractSectionName(section)
-		comment := sectionComment(sectionName, cfg)
-		if comment != "" {
-			b.WriteString(comment)
-			b.WriteString("\n")
-		}
-
-		b.WriteString(section)
-		
-		// Add separator between sections (but not after last)
-		if i < len(sections)-1 {
-			b.WriteString("\n\n")
-		}
-	}
+	// Walk line-by-line, injecting a comment block before each [section] header.
+	for _, line := range strings.Split(tomlStr, "\n") {
+		trimmed := strings.TrimSpace(line)
+		if strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]") &&
+			!strings.HasPrefix(trimmed, "[[") {
+			name := strings.Trim(trimmed, "[]")
+			if comment := sectionComment(name, cfg); comment != "" {
+				b.WriteString("\n")
+				b.WriteString(comment)
+				b.WriteString("\n")
+			}
+		}
+		b.WriteString(line)
+		b.WriteString("\n")
+	}

Also worth adding a test that feeds a config whose serialized output does not contain double newlines between sections, to guard against silent regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/save.go` around lines 38 - 71, Replace the brittle
chunk-based injection that splits tomlStr by "\n\n" with a line-oriented scan
that detects TOML table headers and injects comments immediately before each
header; update the logic around tomlStr/tomlBytes in save.go so instead of using
strings.Split(..., "\n\n") you iterate over strings.Split(tomlStr, "\n") (or
bufio.Scanner) and when you detect a section header (use the same parsing helper
extractSectionName to extract the name) call sectionComment(sectionName, cfg)
and write the comment then the header line into the strings.Builder; ensure you
still preserve blank lines and other non-header lines verbatim and avoid
dropping a trailing newline, and add a unit test that serializes a config with
varying internal blank lines/inline-tables to validate comment placement.

cmd/airlock/cli/wizard/questions.go (2)

32-36: SourceInfo is dead code.

The struct is declared but never constructed, returned, or referenced anywhere in the wizard package. Either wire it into DeriveSandboxName (e.g., return a SourceInfo so callers can know whether the source was GitHub) or remove it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/airlock/cli/wizard/questions.go` around lines 32 - 36, The SourceInfo
struct is unused dead code; either remove the SourceInfo declaration or wire it
into the wizard flow by having DeriveSandboxName (or a related function in the
wizard package) return a SourceInfo (or include it in an existing return struct)
so callers can detect IsGitHub and Path; update all call-sites of
DeriveSandboxName to accept and use the new SourceInfo return (or delete
SourceInfo and remove any vestigial references), and run tests/build to ensure
no unused symbol remains.

---

363-370: Duplicated TTY detection.

wizard.IsTTY() and cli.isTerminal() (in cmd/airlock/cli/cli.go:104-110) are identical in behavior. Additionally, IsTTY() is exported but not called from anywhere in this PR that I can see. Consider either exporting a single helper from one place (e.g., a small shared cli/ttyutil package) and having both call sites use it, or dropping IsTTY if the wizard never needs to branch on it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/airlock/cli/wizard/questions.go` around lines 363 - 370, The IsTTY
function duplicates cli.isTerminal; either remove the unused exported IsTTY or
consolidate both call sites to one shared helper: create a small package (e.g.,
ttyutil) exporting a single function (e.g., IsTTY or IsTerminal), move the
existing logic there, update callers in wizard and cli to import and call that
helper, and delete the duplicate IsTTY/cli.isTerminal implementations; ensure
you update imports and keep the exported name consistent with existing usage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/bootstrap/bootstrap_test.go (1)

55-64: ⚠️ Potential issue | 🟡 Minor

Avoid the QF1011 lint failure while keeping the interface contract check.

The explicit typed blank identifier declarations are flagged by static analysis. A typed no-op function preserves the same assignment checks without tripping the lint rule.

🔧 Proposed fix

-	var (
-		_ api.SandboxManager    = deps.Manager
-		_ api.Provider          = deps.Provider
-		_ api.Provisioner       = deps.Provisioner
-		_ api.ShellProvider     = deps.Sheller
-		_ api.MountManager      = deps.Mounts
-		_ api.NetworkController = deps.Network
-		_ api.ProfileRegistry   = deps.Profiles
-		_ api.RuntimeDetector   = deps.Detector
-	)
+	requireInterfaces := func(
+		api.SandboxManager,
+		api.Provider,
+		api.Provisioner,
+		api.ShellProvider,
+		api.MountManager,
+		api.NetworkController,
+		api.ProfileRegistry,
+		api.RuntimeDetector,
+	) {
+	}
+	requireInterfaces(
+		deps.Manager,
+		deps.Provider,
+		deps.Provisioner,
+		deps.Sheller,
+		deps.Mounts,
+		deps.Network,
+		deps.Profiles,
+		deps.Detector,
+	)

Verify the lint pattern is gone:

#!/bin/bash
rg -n -C2 '^\s*_\s+api\.[A-Za-z]+\s*=\s*deps\.' internal/bootstrap/bootstrap_test.go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/bootstrap/bootstrap_test.go` around lines 55 - 64, The typed blank
identifier interface checks (e.g., _ api.SandboxManager = deps.Manager, _
api.Provider = deps.Provider, etc.) trigger QF1011; replace them with a typed
no-op helper that accepts the concrete values to enforce compile-time assignment
without using typed blank identifiers — add a small function like
assertImplementsSandboxManager(provider api.SandboxManager) and call it with
deps.Manager (and similarly for api.Provider with deps.Provider, api.Provisioner
with deps.Provisioner, api.ShellProvider with deps.Sheller, api.MountManager
with deps.Mounts, api.NetworkController with deps.Network, api.ProfileRegistry
with deps.Profiles, api.RuntimeDetector with deps.Detector) so the compiler
still checks the interface contracts but the linter no longer flags QF1011.

cmd/airlock/cli/cli.go (1)

860-867: ⚠️ Potential issue | 🟠 Major

Default the wizard Node.js version before provisioning.

Line 861 still passes wizardCfg.VM.NodeVersion directly. If the wizard config mapping has not merged defaults, the provisioner receives 0 and can install/provision with an invalid Node version.

Proposed defensive fix

 				// Add provisioning steps
-				for _, step := range deps.Provisioner.ProvisionSteps(spec.Name, wizardCfg.VM.NodeVersion) {
+				nodeVersion := wizardCfg.VM.NodeVersion
+				if nodeVersion <= 0 {
+					nodeVersion = config.Defaults().VM.NodeVersion
+				}
+				for _, step := range deps.Provisioner.ProvisionSteps(spec.Name, nodeVersion) {

Run this read-only check to confirm whether WizardResult.ToConfig already sets a non-zero default:

#!/bin/bash
# Verify whether wizard config mapping populates VM.NodeVersion/defaults.
sed -n '180,260p' cmd/airlock/cli/wizard/mappings.go
rg -n -C4 'NodeVersion|func .*ToConfig|Defaults' cmd/airlock/cli/wizard/mappings.go internal/config

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/airlock/cli/cli.go` around lines 860 - 867, The loop passes
wizardCfg.VM.NodeVersion directly to deps.Provisioner.ProvisionSteps which can
be zero if defaults weren't merged; ensure a non-zero Node.js version is used by
normalizing wizardCfg.VM.NodeVersion before calling ProvisionSteps (e.g., if
wizardCfg.VM.NodeVersion == 0 then set it to the agreed default constant or
derive it from WizardResult.ToConfig/defaults) so ProvisionSteps always receives
a valid Node version; update the code that builds phases (the block using
deps.Provisioner.ProvisionSteps and wizardCfg.VM.NodeVersion) to compute and
pass the normalizedVersion instead of the raw value.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes